Add support for compressed QMDL by wgreenberg · Pull Request #970 · EFForg/rayhunter

wgreenberg · 2026-03-31T02:59:48Z

this is prerequisite work for #81, since the diag logs for things like RSSI massively increase the size of QMDL files. in my experience, simply gzipping the qmdls reduces their size by 4-5x, which i think should be sufficient for our purposes.

this PR reworks QmdlWriter to output gzipped QMDL files by default, and allows QmdlReader to operate on either compressed or uncompressed QMDLs.

QmdlReader has been significantly rewritten to expose a single AsyncRead interface to both compressed and uncompressed QMDL sources.

i'd still like to do some more in-depth testing of this, but in the meantime i'd love a review on it

bmw · 2026-04-03T20:04:00Z

-            self.total_written += msg.data.len();
+            // for a gzipped file, we can't use `msg.data.len()` to
+            // determine the number of bytes written, so we have to
+            // manually do a `write_all()` type loop


i'm not understanding this and i'm hoping you can help me

if what we're tracking here is the number of uncompressed bytes, why can't we still use write_all and msg.data.len()?

ah you're totally right, this is an outdated comment/implementation from when i was trying to track total_bytes_written rather than total_uncompressed_bytes

ah ok. that makes sense

i do want to flag tho that when i try changing this back to use write_all, the tests fail so there may be another reason to use this approach which i also don't understand right now lol

i wager it's due to a missing .flush() call -- i'll push a commit w/ working tests in a sec

bmw

i didn't make the time to really test this (at least not yet), but i wanted to share the comments i had after reading the code and playing with things just a little bit so you could continue working on this

for my benefit at least as much as yours for when i come back to this, my comment above about continuing to use write_all in write_container has not been fully addressed yet

wgreenberg · 2026-06-04T02:22:41Z

@bmw @untitaker sorry about the protracted delay, but i've substantially reworked this based on some of your comments. since you last reviewed it, here are the major changes:

renamed the confusing/redundant names in lib/src/qmdl.rs
reworked QmdlMessageReader (formerly QmdlReader) to automatically detect a compressed file based on the first couple bytes. we can do this safely since no diag container DataType value could equal Gzip's gargantuan magic number 0x1f8b
reworked the QMDL reading process to rely on valid HDLC frames rather than reading up to some boundary. this greatly simplifies a lot of our code that reads QMDL files, removes the confusing bookkeeping of compressed/uncompressed file sizes, and i think overall gives us more robust file reading
reworked QmdlMessageReader's API to be oriented around diag Messages instead of MessagesContainers. the old way existed because long ago this code shared a trait with the DiagDevice struct, so it had to pretend it was reading the MessagesContainer structs when both it and its consumers only care about Messages

i'll mark your previous review comments as resolved since i believe i addressed them all. thanks for your patience!

wgreenberg · 2026-06-04T02:25:24Z

oops, lemme resolve the merge conflicts first

Major changes: * QmdlWriter now outputs gzipped QMDL files by default * QmdlReader renamed to QmdlMessageReader, and reads both compressed and uncompressed QMDL. It no longer requires bounding to avoid reading partially written files.

wgreenberg · 2026-06-04T03:20:49Z

sorry for the force push, but rebasing with so many old commits that i majorly reworked was just untenable. this is review-ready again!

bmw · 2026-06-04T16:59:17Z

just posting this in case it helps you to see it, but it looks like clippy is still mad here

wgreenberg · 2026-06-04T17:04:12Z

bleh, i guess my clippy was slightly outdated and missed a suggestion.

bmw

i still want to manually test this a bit more before merging, but i think we're getting close!

nice work here wgreenberg. this strikes me as a tricky and thorny project

bmw · 2026-06-05T17:21:50Z

    }
 }

+impl From<MessagesContainer> for Bytes {


do we need or want this? when i delete this and the use bytes::Bytes; above and run tests on my fork everything passes

good catch, i think this is leftover from either a test i deleted, or possibly from serializing to an axum Body. in any case, you're right that it's superfluous

bmw · 2026-06-08T19:01:28Z

+            self.writer.flush().await?;
        }


a small thing i noticed playing with GzipEncoder is the compression gets better if you call flush less often. because of that, what do you think about making a change like this?

Suggested change

self.writer.flush().await?;

}

}

self.writer.flush().await?;

i played with this change modifying test_compressed_reading_and_writing to assert the size of the buffer. in the version in this PR, the buffer is 173 or 183 bytes depending on the value of do_close. after making the modification here, it's 146 or 156 bytes

that's not a huge savings, but it's something and i feel like we might as well. what do you think?

deleting this causes run_compressed_reading_and_writing_tests(false) to fail for me, does it not for you?

it seems like when the writer isn't close()'d, without flushes we don't successfully persist all the messages to the disk. i'm not sure how regularly flushing happens if it's not called manually, but i do worry about losing unflushed messages on a crash

to be clear, i'm not proposing we remove the flush calls. i'm just proposing we move it out of the loop to give GzipEncoder a little more context to work with before flushing. the tests still pass for me after applying my suggestion here

this is definitely a nitpick tho so if you don't like this idea for some reason, feel free to ignore it

oh i totally misread your initial comment, sorry! i'm happy to make the change, though fwiw in production i think pretty much all MessagesContainers are of length 1

bmw · 2026-06-08T22:14:44Z

+                hdlcs.iter().map(|hdlc| hdlc.data.len()).collect(),
+            )
+        };
+        for truncated_hdlc_i in 1..hdlcs.len() - 1 {


any reason not to do one more iteration here? the tests still pass with this change

Suggested change

for truncated_hdlc_i in 1..hdlcs.len() - 1 {

for truncated_hdlc_i in 1..hdlcs.len() {

nope, just an off-by-one on my part

bmw · 2026-06-09T16:10:47Z


    let headers = [
        (CONTENT_TYPE, "application/octet-stream"),
        (CONTENT_LENGTH, &entry.qmdl_size_bytes.to_string()),


i think this content length is going to be wrong because qmdl_size_bytes is the compressed size

there are multiple ways to fix this, but i think we should just remove this header. it's inclusion comes from #253 and while having a progress bar is nice, i personally think it's more trouble than it's worth here, especially in the initial version of this

bmw · 2026-06-09T16:21:21Z

                // thread.
                if file_kind == FileKind::Qmdl {
-                    copy(&mut file.take(qmdl_size_bytes as u64), &mut entry_writer).await?;
+                    copy(&mut file.take(qmdl_file_size as u64), &mut entry_writer).await?;


at least when trying to make a zip file with the current recording, this will always generate an invalid gzipped file because it's missing the footer. when i try to decompress this file with gzip, i get:

$ gunzip 1781018397.qmdl.gz gzip: 1781018397.qmdl.gz: unexpected end of file gzip: 1781018397.qmdl.gz: uncompress failed

can we use an approach similar to what was done for get_qmdl above and use into_qmdl_stream on the QmdlMessageReader here?

bmw · 2026-06-09T16:28:33Z

-                .close()
-                .await
-                .expect("failed to close analysis writer");
+            match (qmdl_writer.close().await, analysis_writer.close().await) {


calling close causes the qmdl_writer to add the gzip footer but qmdl_size_bytes is never updated. this is especially bad with how qmdl_size_bytes is currently used when generating a zip file because it means every .qmdl.gz file created there will be missing its footer

i also unfortunately don't think that calling qmdl_writer.size() here will work because close() will have shut down the stream. probably the quickest fix would be to check the size of the file on disk here after calling close and calling update_entry_qmdl_size with that value

what do you think?

actually, i ended up going in a slightly different direction, where close() returns the final size of the file

nice! that's even cleaner imo

A few minor refactors, and a more major one that renames RecordingStore's update_entry_qmdl_size to update_current_entry_qmdl_size, since the only time we're ever updating an entry's QMDL size is when it's the current one.

bmw

this lgtm!

on top of reviewing the new commit, i manually played with this a bit more and couldn't find a way to break it

untitaker mentioned this pull request Apr 1, 2026

GPS logging capability #971

Merged

7 tasks

bmw reviewed Apr 3, 2026

View reviewed changes

bmw reviewed Apr 7, 2026

View reviewed changes

Comment thread check/src/main.rs Outdated

Comment thread lib/src/qmdl.rs Outdated

Comment thread daemon/src/qmdl_store.rs Outdated

Comment thread lib/src/qmdl.rs Outdated

Comment thread daemon/src/server.rs Outdated

Comment thread daemon/src/server.rs Outdated

untitaker reviewed Apr 8, 2026

View reviewed changes

Comment thread lib/src/qmdl.rs Outdated

Comment thread lib/src/qmdl.rs

Comment thread check/src/main.rs Outdated

cooperq assigned bmw Apr 17, 2026

cooperq mentioned this pull request Apr 24, 2026

ROADMAP #990

Open

23 tasks

wgreenberg marked this pull request as ready for review June 4, 2026 02:25

wgreenberg added 2 commits June 3, 2026 20:19

Add support for compressed QMDL

19d9b39

Major changes: * QmdlWriter now outputs gzipped QMDL files by default * QmdlReader renamed to QmdlMessageReader, and reads both compressed and uncompressed QMDL. It no longer requires bounding to avoid reading partially written files.

run cargo fmt

b8fd420

wgreenberg force-pushed the qmdl-gzip branch from f8c9701 to b8fd420 Compare June 4, 2026 03:19

fix clippy

d8c7d9c

more clippy appeasement

cedb94a

bmw requested changes Jun 9, 2026

View reviewed changes

Refactors in response to review comments

7f9ea8e

A few minor refactors, and a more major one that renames RecordingStore's update_entry_qmdl_size to update_current_entry_qmdl_size, since the only time we're ever updating an entry's QMDL size is when it's the current one.

bmw approved these changes Jun 23, 2026

View reviewed changes

bmw merged commit 9627cec into main Jun 23, 2026
40 checks passed

bmw deleted the qmdl-gzip branch June 23, 2026 02:40

	for truncated_hdlc_i in 1..hdlcs.len() - 1 {
	for truncated_hdlc_i in 1..hdlcs.len() {

Uh oh!

Conversation

wgreenberg commented Mar 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgreenberg commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wgreenberg commented Jun 4, 2026

Uh oh!

wgreenberg commented Jun 4, 2026

Uh oh!

bmw commented Jun 4, 2026

Uh oh!

wgreenberg commented Jun 4, 2026

Uh oh!

bmw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wgreenberg commented Jun 4, 2026 •

edited

Loading